Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Suite2pSegmentationInterface #460

Merged
merged 8 commits into from
Jan 9, 2024
Merged

Add Suite2pSegmentationInterface #460

merged 8 commits into from
Jan 9, 2024

Conversation

garrettmflynn
Copy link
Member

This PR adds the Suite2pSegmentationInterface to the allowed interfaces. Though this currently fails on the test data with the following error when attempting to create a stub preview:

Screenshot 2023-10-17 at 10 05 20 AM

@CodyCBakerPhD I tried updating my test data—but this didn't change anything. Looks like this has to do with something in ROIExtractors?

@garrettmflynn garrettmflynn added the interface Add support for a specific NeuroConv interface / converter label Oct 17, 2023
@garrettmflynn garrettmflynn self-assigned this Oct 17, 2023
@garrettmflynn
Copy link
Member Author

Here's the full error:

[main-process]: [2023-10-17 11:59:41,335] ERROR in app: Exception on /neuroconv/convert [POST]
Traceback (most recent call last):
  File "/Users/garrettflynn/Documents/Github/nwb-guide/pyflask/apis/neuroconv.py", line 89, in post
    return convert_to_nwb(neuroconv_api.payload)
  File "/Users/garrettflynn/Documents/Github/nwb-guide/pyflask/manageNeuroconv/manage_neuroconv.py", line 392, in convert_to_nwb
    converter.run_conversion(
  File "/opt/anaconda3/envs/nwb-guide/lib/python3.9/site-packages/neuroconv/nwbconverter.py", line 160, in run_conversion
    self.add_to_nwbfile(nwbfile_out, metadata, conversion_options)
  File "/opt/anaconda3/envs/nwb-guide/lib/python3.9/site-packages/neuroconv/nwbconverter.py", line 114, in add_to_nwbfile
    data_interface.add_to_nwbfile(
  File "/opt/anaconda3/envs/nwb-guide/lib/python3.9/site-packages/neuroconv/datainterfaces/ophys/basesegmentationextractorinterface.py", line 132, in add_to_nwbfile
    add_segmentation(
  File "/opt/anaconda3/envs/nwb-guide/lib/python3.9/site-packages/neuroconv/tools/roiextractors/roiextractors.py", line 1004, in add_segmentation
    add_plane_segmentation(
  File "/opt/anaconda3/envs/nwb-guide/lib/python3.9/site-packages/neuroconv/tools/roiextractors/roiextractors.py", line 711, in add_plane_segmentation
    data=H5DataIO(segmentation_extractor.get_roi_image_masks().T, **compression_options),
  File "/opt/anaconda3/envs/nwb-guide/lib/python3.9/site-packages/roiextractors/segmentationextractor.py", line 126, in get_roi_image_masks
    return np.stack([self._image_masks[:, :, k] for k in roi_idx_], 2)
  File "/opt/anaconda3/envs/nwb-guide/lib/python3.9/site-packages/roiextractors/segmentationextractor.py", line 126, in <listcomp>
    return np.stack([self._image_masks[:, :, k] for k in roi_idx_], 2)
AttributeError: 'FrameSliceSegmentationExtractor' object has no attribute '_image_masks'

@CodyCBakerPhD
Copy link
Collaborator

Pinging @weiglszonja to see if they've ever seen this error before

Suite2p is currently undergoing some improvements anyway, so maybe best to wait until that's done upstream

@weiglszonja
Copy link

weiglszonja commented Oct 18, 2023 •
edited
Loading

hmm, weird. and this is something that only occurred with the suite2p and not with the other segmentation interfaces in stub mode?

@garrettmflynn
Copy link
Member Author

Yeah you got it. The rest were tested within the same hour, aside from Caiman (which was approved a couple weeks ago)

@weiglszonja
Copy link

weiglszonja commented Oct 23, 2023 •
edited
Loading

I figured it out, before catalystneuro/roiextractors#242 the extractor didn't have an _image_masks attribute, it rather override the base get_roi_image_masks() to return the image masks. When we're using FrameSliceSegmentationExtractor for stubbing however, in retrieving the image masks we fall back to the base get_roi_image_masks() which expects the _image_masks attribute.

https://github.com/catalystneuro/roiextractors/blob/88372b5cc44fca1812755ef414d90af45163a331/src/roiextractors/segmentationextractor.py#L133

I believe the other segmentation extractors all have precalculated _image_masks so this issue only came up here.

@weiglszonja weiglszonja mentioned this pull request Nov 16, 2023
Merged
5 tasks
@garrettmflynn
Copy link
Member Author

Just tested. This works now!

@CodyCBakerPhD CodyCBakerPhD marked this pull request as ready for review January 8, 2024 04:16
@CodyCBakerPhD
Copy link
Collaborator

Can you remove the combined and plane_no arguments? They've been hard deprecated for a while, just looks like we forgot to remove them https://github.com/catalystneuro/neuroconv/blob/main/src/neuroconv/datainterfaces/ophys/suite2p/suite2pdatainterface.py#L65-L66

@garrettmflynn
Copy link
Member Author

Updated!

@CodyCBakerPhD
Copy link
Collaborator

CodyCBakerPhD commented Jan 8, 2024 •
edited
Loading

How interesting - we've been seeing some daily failures on the PyFlask builds but they haven't shown up on a PR trigger until now

Could you try looking into the source issue and fix on another PR? I'll do a bit of investigation on my own too

Ref: https://github.com/NeurodataWithoutBorders/nwb-guide/actions/runs/7451051489/attempts/1?pr=460

@CodyCBakerPhD CodyCBakerPhD enabled auto-merge (squash) January 9, 2024 16:22
@CodyCBakerPhD CodyCBakerPhD merged commit e140dcc into main Jan 9, 2024
10 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the Suite2pSegmentationInterface branch January 9, 2024 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CodyCBakerPhD CodyCBakerPhD CodyCBakerPhD approved these changes

Assignees

@garrettmflynn garrettmflynn

Labels
interface Add support for a specific NeuroConv interface / converter
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@garrettmflynn @CodyCBakerPhD @weiglszonja